Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix file exists but file name does not exist #1216

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Relsoul
Copy link

@Relsoul Relsoul commented Jun 12, 2023

https://www.rfc-editor.org/rfc/rfc1867
file name is an optional field.

eg.

----------------------------052948472259119770453321
Content-Disposition: form-data; name="filepond"
Content-Type: application/octet-stream

Comment on lines +99 to +107
if (!filename) {
// @link https://github.com/mscdex/busboy/blob/master/lib/types/multipart.js
// If the type is 'application/octet-stream', it is also identified as a file
if(mimetype==='application/octet-stream'){
filename = 'unknownFile'
}else{
return fileStream.resume()
}
}
Copy link

@ljwagerfield ljwagerfield Jan 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey team!

I've been thinking about the current implementation of this if statement. It might be beneficial for us to consider allowing undefined filenames. This change would give developers more flexibility in handling cases where a client does or does not provide a filename field. This can be pretty important for certain applications.

I wanted to share an interesting point that @Relsoul brought up regarding RFC 7578. The RFC mentions that the filename in the content disposition is recommended but not mandatory. It states:

For form data that represents the content of a file, a name for the file SHOULD be supplied as well, by using a 'filename' parameter of the Content-Disposition header field.
...
The file name isn't mandatory for cases where the file name isn't available or is meaningless or private

With this in mind, it seems quite likely that developers using Multer would appreciate the ability to handle RFC-compliant uploads in a way that best suits their needs, especially when a filename isn't provided. For example, in our system, we prefer storing NULL in the database for the "original file name" if the uploader doesn't provide it, instead of assigning a default value.

Interestingly, busboy allows for undefined filenames, which is a great feature for flexibility. I think it might be a good idea for multer to adopt a similar approach. What do you all think?

To gain parity with busboy, it would simply be a case of removing this entire if statement, and acknowledging that the originalname field may be undefined to reflect RFC 7578:

Suggested change
if (!filename) {
// @link https://github.com/mscdex/busboy/blob/master/lib/types/multipart.js
// If the type is 'application/octet-stream', it is also identified as a file
if(mimetype==='application/octet-stream'){
filename = 'unknownFile'
}else{
return fileStream.resume()
}
}

@Relsoul
Copy link
Author

Relsoul commented Jan 22, 2024

yep,
Here is a simple demo: https://github.com/Relsoul/upload-demo

In the browser environment, formData.append(name, value, filename);
docs: https://developer.mozilla.org/en-US/docs/Web/API/FormData/append

The filename is optional, but the browser will automatically add a filename for Blob/File type objects. However, in other environments, the filename may not be automatically added. When I use a buffer for direct upload, I might ignore the filename.

In Node Express, this is reflected in that if the filename does not exist, it skips the destination function and filename function.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants